-
-
Notifications
You must be signed in to change notification settings - Fork 727
feat(linter/plugins): Token-related SourceCode APIs (TS ESLint implementation)
#15861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(linter/plugins): Token-related SourceCode APIs (TS ESLint implementation)
#15861
Conversation
SourceCode APIs (TS ESLint implementation)
3e3a5bb to
dd03ef0
Compare
Cameron and I had a bit of an argument about exactly this question! We concluded in the end to keep all the deprecated methods, to maximize compatibility with older plugins, which may take some time to get updated (or in some cases, will never get updated). Our rationale is that most of these methods are just aliases, so no maintenance burden to keep them. The only one which is slightly different from its non-deprecated "brother" is
That's true, but even actively developed projects may use old unmaintained plugins. Note: I added the stubs in #15645. That PR also added tests which illustrate the difference in behavior between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly premature review, as it's still marked as draft, but I'm keen to get it merged so I thought I'd go through it now.
Apart from the comments below, this looks good to me. Sorry for the volume of comments - most are pretty small details.
Once we're happy with getTokens impl, I think we should merge this, and we can add more methods in separate PRs. No need to do the whole API in a single PR.
A couple of points which we can also leave to follow-ups:
-
We should ideally lazy-load
@typescript-eslint/typescript-estreepackage only whengetTokensis first called. -
I assume TS-ESLint's parser also generates a
ScopeManager. If it does, we may as well cache it, to avoid running scope analysis again if plugin doessourceCode.getTokens()followed by gettingsourceCode.scopeManager.
176a184 to
db8abc2
Compare
db8abc2 to
0d4164e
Compare
0d4164e to
bc17b6d
Compare
The former delegates to the latter for parsing and lexing. I decided to use |
faf3560 to
de1f77e
Compare
…pshot (#15906) Contents of this test snapshot depends on the constructor names of scope class instances. This is a little fragile, as they can change depending on how the code is bundled (see #15861 (comment)). At some point, we'll full minify the bundle, at which point class names will be come random gibberish like `e`, `t`, `aZ`, etc. Avoid this problem by outputting `type` field of scope objects instead.
Ah ha that makes perfect sense. I had misunderstood. |
e23e179 to
173e5a9
Compare
9fb3811 to
ab159d9
Compare
09a2693 to
56c73d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| version: 5.2.0 | ||
| typescript: | ||
| specifier: 'catalog:' | ||
| specifier: 5.9.3 |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The TypeScript version has been changed from 'catalog:' to a pinned version 5.9.3. This deviates from the project's dependency management convention of using the catalog for shared dependencies like TypeScript.
While pinning the version may provide stability for the token parsing feature, it could lead to version conflicts or maintenance overhead. Consider documenting the reason for this deviation, or using a catalog entry with a pinned version if TypeScript needs to be locked to 5.9.3 for compatibility with @typescript-eslint/typescript-estree.
| specifier: 5.9.3 | |
| specifier: 'catalog:' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. I don't think a change in typescript's internal parsing behavior should prevent the project from type checking using the new version.
It would be unexpected if tests start failing after a routine tooling update. It's two different concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the right thing to do here is. I see your logic, but also you've chosen not to pin @typescript-eslint/typescript-estree dependency, which seems maybe inconsistent.
But anyway, it's a small point. Let's merge and ponder it later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camc314 Any opinion on this?
apps/oxlint/test/fixtures/sourceCode_token_methods/files/eslint-test-case.js
Show resolved
Hide resolved
d9a4425 to
1edc327
Compare
…pshot (#15906) Contents of this test snapshot depends on the constructor names of scope class instances. This is a little fragile, as they can change depending on how the code is bundled (see #15861 (comment)). At some point, we'll full minify the bundle, at which point class names will be come random gibberish like `e`, `t`, `aZ`, etc. Avoid this problem by outputting `type` field of scope objects instead.
#15946 means we can now simplify the TSDown build config. Previously we had to have 2 separate builds for `index.ts` and `cli.ts`, to avoid `assert*` functions ending up in a shared chunk, and not getting removed by minifier. Now that problem is solved, so we can switch to a single build with 2 entry points. TSDown does create a small extra chunk for its `__toESM` and `__commonJSMin` functions, but I think that's fine. Annoyingly it doesn't seem to be possible to tell TSDown to generate `.d.ts` files only for the `index.js` chunk, so have to delete this pointless files in build script. The point of all this is that `RuleTester` needs to use a ton of code from `plugins.ts`, and we wouldn't want all that code duplicated in both `index` and `plugins` chunks (especially once we're bundling TS-ESlint's parser #15861). Now that code will be shared between the two entry points in a shared chunk, not duplicated.
1edc327 to
2358eee
Compare
2358eee to
c85261d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍾 Champagne for everyone!
I've squashed and rebased on latest main, and pushed a few commits with style nits - hope you don't mind. I haven't made any substantive changes. Will merge as soon as CI passes.
Follow-on after #15861. Just add a comment about a potential future perf optimization.
Follow-on after #15861. Reduce variable assignments by using the final value var as `hi` in the binary search. Also, because we initialize `sliceEnd` before the loop, don't need to check if it's `undefined` later on.
SourceCodeAPIs #14829 (comment)@typescript-eslint/typescript-estreefor tokens. The source code is fully parsed and everything but the tokens and comments is discarded.eslint.Tasks
getTokenssrc-js/plugins/tokens.ts_filenameusage in ES modules.Prevent class name mangling bytest(linter/plugins): avoid using scope constructor names in test snapshot #15906 (review)tsdown. Could be deferred.eslinttests.Future work
getFirstTokengetFirstTokensgetLastTokengetLastTokensgetTokenBeforegetTokenOrCommentBeforegetTokensBeforegetTokenAftergetTokenOrCommentAftergetTokensAftergetTokensBetweengetFirstTokenBetweengetFirstTokensBetweengetLastTokenBetweengetLastTokensBetweengetTokenByRangeStartisSpaceBetweenisSpaceBetweenTokensDecisions
@typescript-eslint/typescript-estreepeer-depends ontypescript. How should we package it? Currently,typescriptis being made an optional dependency.getTokenOrCommentBefore,getTokenOrCommentAfter, andisSpaceBetweenTokens. These are surface level deprecations: the functionality was merged with other methods (theincludeComments: trueoption) and plugins can migrate with a one line change. I'm guessing we are targeting fairly modern, actively developed projects. Should we expose them?Programrange is different between ESLint and TS-ESLint. Which one should we follow?